I just realized all inputs of this function is ensured to be older or equal to last_root.

Notice the or equal to part, this assertion must be *slot <= last_root for purge_zero_lamport_accounts. But, we don't need this to begin with, as said above.

sorry, why are they guaranteed to be older than last_root?

I can explain this by reading the code from here upwards the call graph.

Firstly, this cleanup_dead_slots(dead_slots, last_root) is only called from handle_reclaims(reclaims).
dead_slots are derived from reclaims by calling remove_dead_accounts() inside handle_reclaims(), which does just simple 1-to-1 storage -> slot mapping. Thus, we just need to explain in terms of reclaims further on.

handle_reclaims() are called only from purge_zero_lamport_accounts()(1) and store_with_hashes() (2). These are the only 2 code paths reaching to this function.

(1) For the purge_zero_lamport_accounts() cod epath, reasoning is simple:

The reclaims is a collection of results of accounts_index.purge(&pubkey), which in turn the sum of get_rooted_entries() for all such zero lamport pubkeys. So, reclaims (thus, dead_slots) are always rooted slots only. This is true for normal operation because the validator calls add_root() as it roots slots.

Also this is also true for snapshot restoration because generate_index() is called before purge_zero_lamport_accounts() and generate_index() does accounts_index.roots.insert(*slot_id).

(2) For the store_with_hashes(), reasoning is a bit longer:

The reclaims is ultimately only generated at accounts_index.update(). There is intermediately fns like accounts_db.update_index() and accounts_index.insert(), but the destination of this call graph is same.

Next, accounts_index.update(slot, ...) creates reclaims by concatenating two sets:
A: can_purge-able (= slot < max_root) entries
B: Old entries in slot by replacing with updated one

A is obviously fine because max_root is always member of accounts_index.roots. B is ensured not to be dead at remove_dead_accounts() because the case B is always inserting an updated alive entry into the slot.

So, as a normal operation, cleanup_dead_slots() never removes anything such that slot > last_root.

And leaving this line's retain causes halfway-deallocated state because cleanup_dead_slots() is only called once ever for each given slot. That's because later invocations of remove_dead_accounts() would remove any slots whose storage are already removed.

Thanks for reading a rather long comment. :)

So I think this retain does add little value and complicates code reading with seemingly overlapping and spread responsibility for ensuring purge-able slots at caller-site or at callee-site.

As a second thought, if you're a bit concerned for future changes that might break these assumptions, this PR can be changed to assert! that.

Ok. Thanks for the description. I think it can be removed.

I think removing this assertion is ok to remove last_root, first of all.